Fix: improve extra_params support in NeMoGuardrailsServer#1947
Merged
jmartin-tech merged 2 commits intoJul 24, 2026
Conversation
__init__ ran:
if self.extra_params and not self.extra_params.get("extra_body"):
self.extra_params.append("extra_body")
but extra_params is a dict (OpenAICompatible default {}, consumed via
.items()), so .append raises AttributeError. Any user who sets a non-empty
extra_params without an 'extra_body' key crashes generator construction.
The block is also vestigial: guardrails config is delivered through the
self.extra_body attribute (set just below and passed to the client by the
base class). And a naive 'fix' of setting extra_params["extra_body"] = {}
would be worse -- OpenAICompatible._call_model spreads extra_params into the
request after applying self.extra_body, so it would overwrite the real
guardrails config with an empty dict. Since the block only ever crashes or
is skipped (it never completed successfully), removing it changes no working
behaviour. Existing rail-selection tests still pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Andrew Chen <48723787+chuenchen309@users.noreply.github.com>
jmartin-tech
requested changes
Jul 13, 2026
Addresses review feedback from @jmartin-tech: the prior commit removed the crashing `self.extra_params.append("extra_body")` guard outright, but that left a separate, pre-existing bug in place -- a user-provided `extra_body` arriving nested inside `extra_params` was never merged into `self.extra_body`. Since `OpenAICompatible._call_model()` applies `self.extra_params` *after* the instance's own attributes, a lingering `extra_params["extra_body"]` would silently overwrite the guardrails config injected into `self.extra_body`, with the user's own extra_body also never actually reaching the request in combination with it. Pop `extra_body` out of `extra_params` (if present) and fold it into `self.extra_body` before the existing guardrails-merge logic runs, so the two sources combine instead of one shadowing the other. Also raise a clear ValueError if the popped value isn't a dict, per review suggestion, rather than failing confusingly later. Added two tests: one confirms a user extra_body and the guardrails config both end up in the actual outgoing request body (verified via the existing respx-mocked OpenAI client, per review suggestion to check how these map into the client call); the other confirms a non-dict extra_body raises ValueError at construction time. Confirmed both fail against the pre-fix code and pass after. Ran the full generators/test_guardrails.py + test_openai.py suites (no regressions) and black --check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrew Chen <48723787+chuenchen309@users.noreply.github.com>
chuenchen309
force-pushed
the
fix/guardrails-extra-params-crash
branch
from
July 14, 2026 00:54
2271cc2 to
814f5cc
Compare
Contributor
Author
|
Thanks for the detailed review, @jmartin-tech — you're right, removing the guard outright just traded the crash for a silent shadowing bug. Pushed a fix that pops Added a test that verifies this end-to-end through the mocked OpenAI client (checking the actual outgoing request body has both the user's custom key and the guardrails config merged), plus a test for the ValueError on a non-dict |
jmartin-tech
approved these changes
Jul 14, 2026
jmartin-tech
left a comment
jmartin-tech
left a comment
Collaborator
There was a problem hiding this comment.
Latest revision looks good, the unit tests help clarify the expected contract.
jmartin-tech
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NeMoGuardrailsServer.__init__runs:extra_paramsis a dict (inherited fromOpenAICompatible, default{}, consumed via.items()), so.appendraisesAttributeError: 'dict' object has no attribute 'append'. Any user who sets a non-emptyextra_paramswithout anextra_bodykey crashes generator construction:Why remove rather than fix the
.appendself.extra_bodyattribute set just below (and passed to the client by the base class), not via anextra_params["extra_body"]key.self.extra_params["extra_body"] = {}would be worse:OpenAICompatible._call_modelspreadsextra_paramsinto the request after applyingself.extra_body, so it would overwrite the real guardrails config with an empty dict.Verification
python -m pytest tests/test_nonempty_extra_params_does_not_crash_init: constructingNeMoGuardrailsServerwithextra_params={"foo": 1}must not raise and must leaveextra_paramsintact. Fails before the change (AttributeError), passes after.black --checkclean.garak -t <target_type> -n <model_name>— not run: this path needs a live NeMo Guardrails server, which I don't have. The crash is in__init__and is covered by the unit test above; flagging the gap rather than ticking a box I didn't exercise.Disclosure: this contribution was prepared with the assistance of an AI agent (Claude Code). The analysis, fix, and test were reviewed by me before submission.